Conversation
select_first_worker returned the first registered worker without checking its health state, so /v1/models and other proxy GETs returned 500 when the first worker was temporarily down. Filter for a healthy worker instead; fall back to the existing error when none are healthy. Fixes vllm-project#135 Signed-off-by: Binbin Zhang <binbin36520@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Worker selection still bypasses open circuit breakers despite the proxy path requiring an available worker.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates proxy GET routing to avoid unhealthy workers.
Changes:
- Selects a healthy worker for proxy GET endpoints.
- Adds tests for partial and complete worker unavailability.
File summaries
| File | Description |
|---|---|
src/routers/http/router.rs |
Filters worker selection by health and tests fallback behavior. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+344
to
+346
| match workers.into_iter().find(|w| w.is_healthy()) { | ||
| Some(worker) => Ok(worker.url().to_string()), | ||
| None => Err("No healthy workers are available".to_string()), |
Comment on lines
+341
to
+342
| // Prefer a healthy worker so proxy GETs (e.g. /v1/models) don't 500 | ||
| // when the first registered worker is temporarily down. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #135 —
/v1/models(and other proxy GET endpoints) returns 500 when any worker is unhealthy.select_first_workerreturned the first registered worker without checking health state.worker_registry.get_all()returns all workers including unhealthy ones, so when the first worker was temporarily down,proxy_get_requestproxied to a dead worker and returned 500. This affected/v1/models,/get_server_info, and/get_model_info— all of which route throughproxy_get_request→select_first_worker.Root-cause fix in the shared helper: filter for a healthy worker (
is_healthy()) before returning; fall back to the existing error path when none are healthy.Test Plan
New unit tests in
routers::http::router::tests:test_select_first_worker_skips_unhealthy— marks worker1 unhealthy, asserts worker2 is selectedtest_select_first_worker_all_unhealthy— marks all unhealthy, asserts errorTest Result
Built with
cargo test --lib routers::http::router::tests::test_select_first_worker(rust:latest container, aarch64).